-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding toroidal harmonic notebooks #3681
base: feature/toroidal-harmonics
Are you sure you want to change the base?
Adding toroidal harmonic notebooks #3681
Conversation
Quality Gate passedIssues Measures |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/toroidal-harmonics #3681 +/- ##
==============================================================
- Coverage 76.48% 76.02% -0.47%
==============================================================
Files 230 231 +1
Lines 26767 26795 +28
==============================================================
- Hits 20474 20372 -102
- Misses 6293 6423 +130 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my naive understand looks really good. Could have have some tests for the function going into the main package please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments from old merge request:
- add tests for new functions (and check assumed validity domain).
- a reference for Olver.
Otherwise, I have just added a couple of comments on the markdown text in your notebooks.
Am_sin[m] = A_m * np.sin(m * sigma_c) | ||
|
||
# %% [markdown] | ||
# Now we can use the following |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have written up the markdown equations in the order you use them, however, it might be a bit clearer if you introduce us to the TH equations at the start of the notebook so the reader understands what you are aiming for, before breaking it down into the calculation steps.
# | ||
# $$ \psi_{cos} = R \sqrt{\cosh (\tau) - \cos (\sigma)} Q_{m - \frac{1}{2}}^1 | ||
# (\cosh \tau) \cos (m \sigma) $$ | ||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to add a brief note hear about order and degree
I was just running our tests on develop and we get a warning from one of the conversion functions (from arccos of a number outside the range bluemira/bluemira/utilities/tools.py Line 867 in fcce20f
to np.clip((d_1**2 + d_2**2 - 4 * R_0**2) / (2 * d_1 * d_2), -1, 1) this seem to remove the warning in my quick test. I think its just floating point problems. |
Linked Issues
(This merge request is updated version of #3645)
Closes #{ID}
Description
Interface Changes
Checklist
I confirm that I have completed the following checks:
pytest tests --reactor
pre-commit run --from-ref develop --to-ref HEAD
sphinx-build -W documentation/source documentation/build